Claude finished @ovr's task in 2m 52s —— View job

Code Review: feat(client-core): Improve format API

• Gather context and read changed files
• Review code quality and best practices
• Check for security and performance issues
• Evaluate test coverage
• Post review feedback

Summary

This PR makes three improvements to @cubejs-client/core:

1. Sub-path export for ./format — Exposes formatValue via @cubejs-client/core/format for ESM consumers, avoiding CJS/UMD bundle bloat (aligning with the existing comment at src/index.ts:914).
2. currency + granularity in TableColumn — Carries these annotation fields through tableColumns() so consumers can use them for formatting without re-parsing annotations.
3. Boolean type handling in formatValue — Coerces boolean and numeric (0/1) values when type === 'boolean', addressing SQL driver inconsistencies.

Code Quality ✅

• Clean, well-structured changes. The extractFields destructuring in ResultSet.ts:920 is a natural extension.
• granularity: granularity?.name correctly unwraps the GranularityAnnotation object to a plain string, matching the TableColumn type contract.
• JSDoc comments on the new TableColumn fields are clear and helpful.
• The exports + typesVersions setup in package.json follows the recommended Node.js sub-path export pattern.

Security ✅

No security concerns. The changes are purely formatting/presentation logic with no user input reaching execution paths.

Performance ✅

No performance concerns. The boolean check adds a negligible branch at the top of formatValue. The currency/granularity extraction is a simple destructuring addition.

Testing 🟡

• The boolean coercion test covers true/false, 1/0, and 'true'/'false' strings — good coverage of the common SQL driver representations.
• ResultSet.test.ts snapshot updates correctly verify the new fields propagate through tableColumns().
• Gap: String-encoded '0'/'1' values (which some SQL drivers produce) hit the String(value) fallback and return '0'/'1' instead of 'false'/'true'. This may or may not be intentional — see inline comment with a suggestion.
• Minor: The formatValue(true, { type: 'boolean' }) assertion is duplicated between the existing "default fallback" test (line 99) and the new "boolean: coerces numeric 0/1" test (line 104).
• I was unable to run the test suite due to tool permission constraints, so I recommend verifying all tests pass in CI.

Overall

Clean, focused PR. The sub-path export is a good pattern for keeping the CJS bundle lean. The currency/granularity passthrough and boolean formatting are straightforward and well-tested. I left a few minor inline suggestions — nothing blocking.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.03%. Comparing base (9ba2897) to head (24a186e).
⚠️ Report is 2 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (9ba2897) and HEAD (24a186e). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (9ba2897)	HEAD (24a186e)
cubesql	1	0

@@             Coverage Diff             @@
##           master   #10692       +/-   ##
===========================================
- Coverage   78.80%   58.03%   -20.78%     
===========================================
  Files         465      215      -250     
  Lines       91944    16683    -75261     
  Branches     3357     3357               
===========================================
- Hits        72458     9682    -62776     
+ Misses      18996     6511    -12485     
  Partials      490      490               

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.